Skip to content

Conversation

@agoscinski
Copy link
Owner

No description provided.

agoscinski and others added 10 commits June 11, 2025 14:21
…|play|pause}` (aiidateam#6902)

The original design of `wait` and `timeout` was to distinguish between actions
that immediately return and actions that scheduled. This mechanism was however
never used and resulted in an misinterpretation in the force-kill PR aiidateam#6793
introducing a bug fixed in PR $6870. The mechanism of `wait` and `timeout` was
also never correctly implemented. In this PR we rectify the logic and simplify
it by handling immediate and scheduled actions the same way.

My interpretation is that the original logic (seen in commit 8388018) is that it
unwraps the future once, if it is a another future then it is a scheduled action
otherwise an immediate one. Then 1b6ecb8 in plumpy introduced a double wrapping
for `play`, `pause` and `kill` of the return value which I think is correct as
these are scheduled actions. The problem appears in cd0d15c where on the
aiida-core side in the first round unwrapping is always done till the actual
(nonfuture) result by using `unwrap_kiwi_future`. This makes the usage of
`wait` completely obsolete as we always get the final nonfuture result in the
first step. Also the `timeout` was not passed to the `result` which made it a
blocking action for scheduled tasked (the `timeout` is only applied on the
first layer of future in `futures.as_completed` which is meaningless since the
first layer is always a future and returns immediately).

This is fixed by simplifying the two step procedure, unwrap once and if future
unwrap again, to one step: Unwrap until nonfuture result gained with timeout.
One can specify a `timeout == 0` to express that the action should not wait for
a response while one can specify `timeout==float('inf')` (default value) to wait
until a response has been received without a timeout.
This bug was introduced in PR aiidateam#6610, where the temporary config folder
path was changed unintentionally. Previously, it was set to
`/tmp/pytest-.../config/.aiida`, but the new setup used
`/tmp/pytest-.../config/`. As a result, the daemon_client fixture failed
to detect the profile, leading to subtle issues.

This bug was masked by two factors:
 - The CI setup (`setup.sh`) which preconfigures AiiDA profiles.
 - The fixture definitions in `tests/conftest.py`, which interfered with
   the intended isolation of aiida_instance.

A follow-up commit will address this entanglement by clearly separating
fixture configuration and test logic.
Previously, legacy pytest fixtures were only tested via
`tests/manage/tests/test_pytest_fixtures.py`. These tests were
inadvertently affected by:
 - Fixtures in `tests/conftest.py`, which caused test contamination.
 - The CI environment, where AiiDA profiles were already created via
   setup.sh.

To ensure true isolation:
 - The `--no-conftest` flag is now used to prevent loading `conftest.py`.
 - A new CI job in the ci-code workflow has been added to run these
   tests independently.
 - The test file has been relocated to the src directory to avoid
   interference from the main test suite.
Fixes aiidateam#6881. See individual commits for detailed changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants